Skip to content

Conversation

@graingert
Copy link

No description provided.

'flow.drop', 'flow.transfers', 'flow.img', 'flow.dragEvents']);

return 'flow';
}));
Copy link
Author

@graingert graingert Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

300 lines is not enough to warrant splitting this module into multiple files.

/* for module loading using webpack or similar package bundlers */
window.Flow = require('./dist/ng-flow-standalone');
module.exports = 'flow';
/* backwards compatable with deprecated index.js*/
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is ignored by commonjs loaders because of the 'main' key in package.json so I log a warning.

@@ -1,3 +1,7 @@
/* for module loading using webpack or similar package bundlers */
window.Flow = require('./dist/ng-flow-standalone');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutating window in a commonjs module is not acceptable.

@AidasK
Copy link
Member

AidasK commented Jul 6, 2017

@evilaliv3 Can you review this? I haven't used yarn or webpack so I can't test this. But as long as all tests are passing, it is good for me.

I think this will be a breaking change for all of our users. We can release it under a major version.

@graingert
Copy link
Author

@AidasK ok I'll change it so it's not breaking

}

if (typeof window === 'object') {
window.Flow = require('@flowjs/flow.js');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutating window in a commonjs module is not acceptable, but I do it here for require('@flowjs/ng-flow/index') for backwards compatibility reasons.

@AidasK
Copy link
Member

AidasK commented Jul 6, 2017

You have also removed ng-flow.js from dist folder, this is bc.

I am not against of breaking things. There is nothing wrong in releasing a major version if it is needed.

@graingert
Copy link
Author

You have also removed ng-flow.js from dist folder, this is bc.

oh woops, that's an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants